Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a pyproject.toml file. #516

Merged
merged 2 commits into from
Dec 17, 2024
Merged

Conversation

Justin-Hoffman
Copy link
Contributor

@Justin-Hoffman Justin-Hoffman commented Dec 12, 2024

Addresses #487

Remove setup.py and move to pyproject.toml.

Right now this results in duplicate instances of project version ( inside pyproject.toml and inside ./nam/_version.py)
I'm sure there's an elegant way to avoid this, I just haven't done it yet. I wanted to open the MR for feedback as is.

^ This is addressed in the second commit.

With this change you can still just
python3 -m pip install .

Both the workaround for the older transformer/numpy library and test dependencies are expressed as optional dependencies and can be installed with:
python3 -m pip install .[test] (or .[transformer-compat])

But maybe it's time to remove that workaround?

I haven't addressed the conda environments files, but I propose that we put:
- neural-amp-modeler
or possibly:
- neural-amp-modeler[test]
under the dependencies/pip section and remove the requirements.txt (which is redundant because they are in pyproject.toml now)

The environment files can likely get a de-duplication pass as well.

@Justin-Hoffman
Copy link
Contributor Author

Justin-Hoffman commented Dec 12, 2024

I think the workflow would work as is, if you approve it, but for the sake of brevity it might be worth modifying the actions file to only python -m pip install .[test] and remove the line with logic that installs requirements.txt, if it exists (requirements are now in pyproject.toml)

@sdatkinson
Copy link
Owner

Thanks for the PR @Justin-Hoffman. Are you able to see the workflow logs? If so would you mind having a look and fixing?

@Justin-Hoffman
Copy link
Contributor Author

I can see and will take a look.

@Justin-Hoffman
Copy link
Contributor Author

Justin-Hoffman commented Dec 12, 2024

This passes a local run of xvfb-run -a pytest, so I expect this workflow likely will as well (barring any unexpected version related issues).

At risk of polluting the commit I also made nam/_version.py a generated file. The version number is now generated dynamically from existing tags.

Locally my builds end up as version 0.11.1.dev4, because the last tag in the repo is 0.11.0, so the patch is incremented and dev4 is appended because this commit isn't a tag and we're 4 commits past the most recent tag (4 commits past 0.11.0)

@Justin-Hoffman
Copy link
Contributor Author

Bumping this @sdatkinson - Let me know if there's anything else you'd like to see here.

@sdatkinson
Copy link
Owner

Sorry for the delay. I've been busy this week. Going to find time in the coming week.

Copy link
Owner

@sdatkinson sdatkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏻

@sdatkinson sdatkinson merged commit eaf9a86 into sdatkinson:main Dec 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants